Redesign how the control bar handles fill and stroke colors#4137
Merged
Conversation
Keavon
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the color selection and synchronization system across the editor's toolset. It introduces ToolColorOptions and DrawingToolState to unify how tools handle fill and stroke properties, including support for "mixed" states in the UI when selections have heterogeneous values. Key changes include updated message handling for node deletion and input updates, enhanced utility functions for querying and modifying layer styles, and frontend improvements to ColorInput and CheckboxInput for indeterminate states. Reviewers identified an inefficiency in the selection change detection logic and potential bugs in the aggregation of fill and stroke states when the primary selection lacks those properties.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 18 files
Confidence score: 3/5
- There is meaningful user-facing risk before merge: in
editor/src/messages/tool/tool_messages/brush_tool.rs, storing picked swatches in gamma space instead of linear sRGB can produce visibly inaccurate brush rendering. - Several high-confidence mixed-state handling issues in
editor/src/messages/tool/common_functionality/graph_modification_utils.rsandeditor/src/messages/tool/tool_messages/text_tool.rscan present misleading concrete fill/stroke values when selections are actually indeterminate. - Tool abort behavior appears inconsistent in
editor/src/messages/tool/tool_messages/freehand_tool.rsandeditor/src/messages/tool/tool_messages/gradient_tool.rs, with potential state loss (fill/stroke options reset) or stale defaults persisting after cancel flows. - Pay close attention to
editor/src/messages/tool/tool_messages/brush_tool.rs,editor/src/messages/tool/common_functionality/graph_modification_utils.rs,editor/src/messages/tool/tool_messages/freehand_tool.rs,editor/src/messages/tool/tool_messages/gradient_tool.rs, andeditor/src/messages/tool/tool_messages/text_tool.rs- color-space conversion, mixed-state UI correctness, and abort/reset paths need validation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/tool_messages/freehand_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/freehand_tool.rs:169">
P2: `Abort` now resets drawing colors even during in-tool cancel (e.g. Esc while drawing), which unintentionally wipes the current fill/stroke option state.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/gradient_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/gradient_tool.rs:1560">
P2: `default_gradient_stops` is only cleared in one abort branch; aborting from `Drawing` skips this reset and can leave stale tool-default stops.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/text_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/text_tool.rs:272">
P2: Mixed fill state is lost in the text color control because `fill_choice == None` is coerced to `FillChoice::None` without setting the widget to mixed/indeterminate.</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/graph_modification_utils.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/graph_modification_utils.rs:587">
P2: `fill_choice` should be treated as mixed when fill enabled state is mixed; currently it can return a concrete value in mixed enabled/disabled selections.</violation>
<violation number="2" location="editor/src/messages/tool/common_functionality/graph_modification_utils.rs:618">
P2: `optional_color` should also become indeterminate when stroke enabled state is mixed, otherwise mixed selections can show a misleading concrete color.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/brush_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/brush_tool.rs:111">
P1: Convert the picked swatch color to linear sRGB before storing it as the working color; otherwise brush color values are gamma-space and render inaccurately.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| (_, GradientToolMessage::Abort) => { | ||
| dismiss_color_stop_color_picker(tool_data, responses); | ||
| // Clear the tool-default gradient override so re-activating the tool starts fresh from the working colors | ||
| tool_data.default_gradient_stops = None; |
Contributor
There was a problem hiding this comment.
P2: default_gradient_stops is only cleared in one abort branch; aborting from Drawing skips this reset and can leave stale tool-default stops.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/gradient_tool.rs, line 1560:
<comment>`default_gradient_stops` is only cleared in one abort branch; aborting from `Drawing` skips this reset and can leave stale tool-default stops.</comment>
<file context>
@@ -1545,6 +1556,8 @@ impl Fsm for GradientToolFsmState {
(_, GradientToolMessage::Abort) => {
dismiss_color_stop_color_picker(tool_data, responses);
+ // Clear the tool-default gradient override so re-activating the tool starts fresh from the working colors
+ tool_data.default_gradient_stops = None;
GradientToolFsmState::Ready {
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/widgets/inputs/NumberInput.svelte">
<violation number="1" location="frontend/src/components/widgets/inputs/NumberInput.svelte:251">
P1: A started history transaction can be left uncommitted on drag teardown paths (lost-focus recovery or component destroy), causing an open/stale history transaction.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2356.